Install Xpert chatbot from frontend-lib-learning-assistant. #1166

MichaelRoytman · 2023-08-22T19:08:59Z

Description

This commit installs the Xpert chatbot feature from the frontend-lib-learning-assistant repository into the frontend-lib-learning application.

This component is rendered by the Course component. The component is only rendered when a few conditions are satisfied.

JIRA: MST-2034 (private)

Demo

NOTE: Please ignore the line over the video camera icon. This was fixed in edx/frontend-lib-learning-assistant#12.

MST-2034-Xpert-Demo.mov

MichaelRoytman · 2023-08-22T19:15:58Z

src/courseware/course/Course.jsx

@@ -93,10 +93,10 @@ const Course = ({
        />
        {shouldDisplayTriggers && (
          <>
-            <ChatTrigger
+            <Chat
+              enabled={course.learningAssistantEnabled}


I'd prefer to read these values from the Redux store, but it'll make testing more difficult because I'll have to set up that part of the Redux store, so I've chosen to use props here in the interest of getting this out more quickly.

MichaelRoytman · 2023-08-22T19:16:25Z

src/courseware/course/chat/Chat.jsx

+  enrollmentMode: null,
+};
+
+export default injectIntl(Chat);


I'm not sure if this is really necessary, since there's no text to internationalize right now.

MichaelRoytman · 2023-08-22T19:17:22Z

src/courseware/course/chat/Chat.test.jsx

+
+const courseId = 'course-v1:edX+DemoX+Demo_Course';
+let testCases = [];
+let enabledTestCases = [];


I tried to refactor the test to generate the test cases instead of declaring them outright. This is because the conditions under which the Xpert chatbot should appear have grown more complex, so it's easier to generate the objects. Let me know if this makes it more confusing to read.

MichaelRoytman · 2023-08-22T19:17:34Z

src/courseware/course/chat/Chat.test.jsx

+          { store },
+        );
+
+        // This is rather brittle and depends on the implementation of the Xpert component, but there


Any other ideas here?

Is there an ID that we can query by instead? I'm hesitant to query by button name because that could change

We could add a data-testid property to the button that we could query for. I'm a little torn, because I think we should be querying by roles like button, because it's a more representative and accessible way of testing. And I wonder if these tests should break if we change the text of the button. But, on the other hand, these tests aren't meant to test how the toggle button looks and behaves.

Actually, now that I think of it, since I'm only rendering the Xpert in the test, there should only be a single button in the DOM, so I could get away with this.

const chat = screen.queryByRole('button');

What do you think of that? I feel like it's more reasonable to assume there is a button in the DOM than a button with a particular "name". If you prefer the test ID, I can work on adding that.

If it's only rendering the Xpert Component in the test, I like the idea of querying by the button role alone, as opposed to a button with a particular name. That seems like an appropriate solution to me that is definitely less brittle.

MichaelRoytman · 2023-08-22T19:18:25Z

src/setupTest.js

@@ -116,6 +117,7 @@ export async function initializeTestStore(options = {}, overrideStore = true) {
      models: modelsReducer,
      courseware: coursewareReducer,
      courseHome: courseHomeReducer,
+      learningAssistant: learningAssistantReducer,


I'm a little confused why the learningAssistantReducer is necessary, but the tours or recommendations reducers aren't. Without the learningAssistantReducer, other tests were breaking, so I added it.

alangsto · 2023-08-22T19:45:06Z

src/courseware/course/chat/Chat.jsx

+
+  return (
+    <>
+      {/* Use a portal to ensure that component overlay does not compete with learning MFE styles. */}


Thank you adding this comment!

alangsto · 2023-08-22T19:48:32Z

src/courseware/course/chat/Chat.test.jsx

+          { store },
+        );
+
+        // This is rather brittle and depends on the implementation of the Xpert component, but there


Is there an ID that we can query by instead? I'm hesitant to query by button name because that could change

This commit installs the Xpert chatbot feature from the frontend-lib-learning-assistant repository into the frontend-lib-learning application. This component is rendered by the Course component. The component is only rendered when a few conditions are satisfied.

codecov · 2023-08-22T21:15:49Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (2e90e21) 87.87% compared to head (4ba1cde) 87.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1166      +/-   ##
==========================================
- Coverage   87.87%   87.85%   -0.03%     
==========================================
  Files         264      263       -1     
  Lines        4480     4470      -10     
  Branches     1124     1122       -2     
==========================================
- Hits         3937     3927      -10     
  Misses        529      529              
  Partials       14       14

Files Changed	Coverage Δ
src/courseware/course/Course.jsx	`88.88% <ø> (ø)`
...eware/data/__factories__/courseMetadata.factory.js	`100.00% <ø> (ø)`
src/courseware/data/api.js	`84.61% <ø> (ø)`
src/store.js	`100.00% <ø> (ø)`
src/courseware/course/chat/Chat.jsx	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MichaelRoytman · 2023-08-22T21:20:43Z

@alangsto I addressed your feedback. Let me know what you think!

alangsto

Looks good to me!

arbrandes · 2023-09-01T13:13:22Z

Hey there! This just came to my attention. Is there an OEP, ADR, or other documentation (including any community discussions) that describe the intention behind this new functionality?

MichaelRoytman force-pushed the michaelroytman/MST-2034-install-learning-assistant-lib branch from 1cbe62e to d4333a3 Compare August 22, 2023 19:12

MichaelRoytman commented Aug 22, 2023

View reviewed changes

MichaelRoytman changed the title ~~Install Xpert chatbot from frontend-lib-learning-assistant~~ Install Xpert chatbot from frontend-lib-learning-assistant. Aug 22, 2023

MichaelRoytman force-pushed the michaelroytman/MST-2034-install-learning-assistant-lib branch from d4333a3 to c88cd44 Compare August 22, 2023 19:27

alangsto reviewed Aug 22, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/MST-2034-install-learning-assistant-lib branch from c88cd44 to 4ba1cde Compare August 22, 2023 21:02

alangsto approved these changes Aug 22, 2023

View reviewed changes

MichaelRoytman merged commit 2d29827 into master Aug 23, 2023
6 checks passed

MichaelRoytman deleted the michaelroytman/MST-2034-install-learning-assistant-lib branch August 23, 2023 13:14

feanil mentioned this pull request Sep 1, 2023

Code Specific to 2U should not get hard coded into the Open edX Platform edx/open-source-process-wg#127

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install Xpert chatbot from frontend-lib-learning-assistant. #1166

Install Xpert chatbot from frontend-lib-learning-assistant. #1166

MichaelRoytman commented Aug 22, 2023

MichaelRoytman Aug 22, 2023

MichaelRoytman Aug 22, 2023

MichaelRoytman Aug 22, 2023

MichaelRoytman Aug 22, 2023

alangsto Aug 22, 2023

MichaelRoytman Aug 22, 2023 •

edited

alangsto Aug 22, 2023

MichaelRoytman Aug 22, 2023

alangsto Aug 22, 2023

alangsto Aug 22, 2023

codecov bot commented Aug 22, 2023

MichaelRoytman commented Aug 22, 2023

alangsto left a comment

arbrandes commented Sep 1, 2023

Install Xpert chatbot from frontend-lib-learning-assistant. #1166

Install Xpert chatbot from frontend-lib-learning-assistant. #1166

Conversation

MichaelRoytman commented Aug 22, 2023

Description

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRoytman Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023

Codecov Report

MichaelRoytman commented Aug 22, 2023

alangsto left a comment

Choose a reason for hiding this comment

arbrandes commented Sep 1, 2023

MichaelRoytman Aug 22, 2023 •

edited